Security: enforce auth and ownership checks on chat fallback DELETE /api/scheduled-actions/delete#1640
Conversation
…ation - Implemented header validation and authentication checks in the DELETE route for scheduled actions. - Added Zod schema validation for incoming request body to ensure valid UUID format. - Introduced comprehensive test cases for various scenarios including unauthorized access, invalid IDs, and successful deletion. - Updated deleteTask function to include access token in the request headers for secure deletion. - Enhanced useDeleteScheduledAction hook to retrieve access token before performing delete operation.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe pull request adds authentication and authorization to the scheduled actions deletion flow. The API endpoint now validates requests via headers, parses and validates request bodies using Zod, retrieves the target scheduled action, checks access permissions (ownership or artist access), and returns appropriate error codes. The client-side hook integrates Privy for obtaining access tokens and passes them through the deletion task to the API. Changes
Sequence DiagramsequenceDiagram
actor User
participant Hook as React Hook<br/>(useDeleteScheduledAction)
participant Privy as Privy Auth
participant Client as API Client<br/>(deleteTask)
participant API as DELETE Endpoint<br/>(delete/route.ts)
participant DB as Supabase DB
User->>Hook: Click delete action
Hook->>Privy: getAccessToken()
Privy-->>Hook: accessToken
rect rgba(0, 100, 200, 0.5)
Note over Hook,Client: Token obtained
Hook->>Client: deleteTask({id, accessToken})
Client->>API: DELETE /api/scheduled-actions/delete<br/>Headers: Authorization: Bearer token<br/>Body: {id}
end
rect rgba(200, 100, 0, 0.5)
Note over API,DB: Authorization flow
API->>API: validateHeaders(req)
API->>API: Parse & validate body with Zod
API->>DB: selectScheduledActionById(id)
DB-->>API: scheduledAction{id, account_id, artist_account_id}
alt Owns action or has artist access
API->>DB: deleteScheduledActionById(id)
DB-->>API: Success
API-->>Client: 200 OK
else Unauthorized
API-->>Client: 403 Forbidden
end
end
Client-->>Hook: Success/Error
Hook-->>User: Action deleted or error shown
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd51111506
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| return ( | ||
| <div className={cn("flex flex-col gap-2", { "items-end": message.role === "user" })}> | ||
| <div className="flex flex-col gap-2 items-end"> |
There was a problem hiding this comment.
Preserve left alignment for assistant messages
This change makes items-end unconditional, so assistant text parts now render right-aligned just like user messages. In normal chat flows where both roles are shown, this collapses the visual distinction between user and assistant messages and conflicts with the surrounding layout logic (for example, assistant actions are still left-justified in MessageParts). Restoring the role-based class condition avoids this UI regression.
Useful? React with 👍 / 👎.
| */ | ||
| async function deleteTaskFromDatabase(taskId: string): Promise<void> { | ||
| async function deleteTaskFromDatabase(taskId: string, accessToken: string): Promise<void> { | ||
| await fetch("/api/scheduled-actions/delete", { |
There was a problem hiding this comment.
Fail when fallback DB delete returns non-2xx
The fallback route now enforces auth and ownership checks, but this fetch result is ignored, so deleteTaskFromDatabase resolves even on 401/403/500. In the "Schedule not found" path, that means the caller reports a successful deletion even when the row was not deleted (for example, expired token or revoked access). Please check response.ok and throw on failure so the UI can surface the error instead of silently succeeding.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
4 issues found across 5 files
Confidence score: 3/5
- There is some user-visible risk:
components/VercelChat/ViewingMessage.tsxnow always appliesitems-end, which will right-align assistant messages and could regress chat layout behavior. lib/tasks/deleteTask.tsdoes not check the fetch response, so newly added auth failures (401/403) can be silently ignored, leading to callers thinking deletes succeeded when they did not.- Some smaller UX issue remains where
hooks/useDeleteScheduledAction.tsswallows the specific sign-in prompt and always shows a generic error toast. - Pay close attention to
components/VercelChat/ViewingMessage.tsx,lib/tasks/deleteTask.ts,hooks/useDeleteScheduledAction.ts- alignment regression, silent delete failures, and masked auth messaging.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/api/scheduled-actions/delete/__tests__/route.test.ts">
<violation number="1" location="app/api/scheduled-actions/delete/__tests__/route.test.ts:1">
P2: Custom agent: **Code Structure and Size Limits for Readability and Single Responsibility**
This test file is 129 lines, exceeding the 100-line limit. Extract the mock setup and `makeRequest` helper (lines 1–29) into a shared test-utils file to bring it under the threshold.</violation>
</file>
<file name="lib/tasks/deleteTask.ts">
<violation number="1" location="lib/tasks/deleteTask.ts:22">
P1: The fetch response in `deleteTaskFromDatabase` is never checked. With the newly added `Authorization` header, the server can now reject the request (401/403), but this failure is silently ignored — the caller returns as if deletion succeeded. Check `response.ok` and throw on failure so the error propagates to the user.</violation>
</file>
<file name="components/VercelChat/ViewingMessage.tsx">
<violation number="1" location="components/VercelChat/ViewingMessage.tsx:29">
P1: `items-end` is now applied unconditionally, so assistant messages will be right-aligned like user messages. The original conditional `{ "items-end": message.role === "user" }` was intentional — `ViewingMessage` is rendered for both roles (see `MessageParts.tsx` line 64). This reverts the correct chat bubble alignment.</violation>
</file>
<file name="hooks/useDeleteScheduledAction.ts">
<violation number="1" location="hooks/useDeleteScheduledAction.ts:27">
P2: The auth error message "Please sign in to delete scheduled actions" is swallowed by the catch block's generic `toast.error("Failed to delete. Please try again.")`. The user never sees the specific sign-in prompt. Consider using the error's message in the toast for auth failures, e.g. `toast.error(error instanceof Error ? error.message : "Failed to delete. Please try again.")`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| async function deleteTaskFromDatabase(taskId: string, accessToken: string): Promise<void> { | ||
| await fetch("/api/scheduled-actions/delete", { | ||
| method: "DELETE", | ||
| headers: { "Content-Type": "application/json" }, | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| Authorization: `Bearer ${accessToken}`, | ||
| }, | ||
| body: JSON.stringify({ id: taskId }), |
There was a problem hiding this comment.
P1: The fetch response in deleteTaskFromDatabase is never checked. With the newly added Authorization header, the server can now reject the request (401/403), but this failure is silently ignored — the caller returns as if deletion succeeded. Check response.ok and throw on failure so the error propagates to the user.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/tasks/deleteTask.ts, line 22:
<comment>The fetch response in `deleteTaskFromDatabase` is never checked. With the newly added `Authorization` header, the server can now reject the request (401/403), but this failure is silently ignored — the caller returns as if deletion succeeded. Check `response.ok` and throw on failure so the error propagates to the user.</comment>
<file context>
@@ -18,10 +19,13 @@ function isScheduleNotFoundError(errorText: string): boolean {
* Delete task record from database when scheduler deletion isn't possible
*/
-async function deleteTaskFromDatabase(taskId: string): Promise<void> {
+async function deleteTaskFromDatabase(taskId: string, accessToken: string): Promise<void> {
await fetch("/api/scheduled-actions/delete", {
method: "DELETE",
</file context>
| async function deleteTaskFromDatabase(taskId: string, accessToken: string): Promise<void> { | |
| await fetch("/api/scheduled-actions/delete", { | |
| method: "DELETE", | |
| headers: { "Content-Type": "application/json" }, | |
| headers: { | |
| "Content-Type": "application/json", | |
| Authorization: `Bearer ${accessToken}`, | |
| }, | |
| body: JSON.stringify({ id: taskId }), | |
| async function deleteTaskFromDatabase(taskId: string, accessToken: string): Promise<void> { | |
| const response = await fetch("/api/scheduled-actions/delete", { | |
| method: "DELETE", | |
| headers: { | |
| "Content-Type": "application/json", | |
| Authorization: `Bearer ${accessToken}`, | |
| }, | |
| body: JSON.stringify({ id: taskId }), | |
| }); | |
| if (!response.ok) { | |
| const errorText = await response.text(); | |
| throw new Error(`Failed to delete task from database: HTTP ${response.status}: ${errorText}`); | |
| } | |
| } |
|
|
||
| return ( | ||
| <div className={cn("flex flex-col gap-2", { "items-end": message.role === "user" })}> | ||
| <div className="flex flex-col gap-2 items-end"> |
There was a problem hiding this comment.
P1: items-end is now applied unconditionally, so assistant messages will be right-aligned like user messages. The original conditional { "items-end": message.role === "user" } was intentional — ViewingMessage is rendered for both roles (see MessageParts.tsx line 64). This reverts the correct chat bubble alignment.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At components/VercelChat/ViewingMessage.tsx, line 29:
<comment>`items-end` is now applied unconditionally, so assistant messages will be right-aligned like user messages. The original conditional `{ "items-end": message.role === "user" }` was intentional — `ViewingMessage` is rendered for both roles (see `MessageParts.tsx` line 64). This reverts the correct chat bubble alignment.</comment>
<file context>
@@ -26,7 +26,7 @@ const ViewingMessageComponent: React.FC<ViewingMessageProps> = ({
return (
- <div className={cn("flex flex-col gap-2", { "items-end": message.role === "user" })}>
+ <div className="flex flex-col gap-2 items-end">
{hasAttachments && (
<div className="flex flex-wrap gap-2">
</file context>
| <div className="flex flex-col gap-2 items-end"> | |
| <div className={cn("flex flex-col gap-2", { "items-end": message.role === "user" })}> |
| await deleteTask({ id: actionId }); | ||
| const accessToken = await getAccessToken(); | ||
| if (!accessToken) { | ||
| throw new Error("Please sign in to delete scheduled actions"); |
There was a problem hiding this comment.
P2: The auth error message "Please sign in to delete scheduled actions" is swallowed by the catch block's generic toast.error("Failed to delete. Please try again."). The user never sees the specific sign-in prompt. Consider using the error's message in the toast for auth failures, e.g. toast.error(error instanceof Error ? error.message : "Failed to delete. Please try again.").
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At hooks/useDeleteScheduledAction.ts, line 27:
<comment>The auth error message "Please sign in to delete scheduled actions" is swallowed by the catch block's generic `toast.error("Failed to delete. Please try again.")`. The user never sees the specific sign-in prompt. Consider using the error's message in the toast for auth failures, e.g. `toast.error(error instanceof Error ? error.message : "Failed to delete. Please try again.")`.</comment>
<file context>
@@ -20,7 +22,12 @@ export const useDeleteScheduledAction = () => {
- await deleteTask({ id: actionId });
+ const accessToken = await getAccessToken();
+ if (!accessToken) {
+ throw new Error("Please sign in to delete scheduled actions");
+ }
+
</file context>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/api/scheduled-actions/delete/route.ts (1)
37-63:⚠️ Potential issue | 🔴 CriticalMake the authorization check and delete atomic.
This handler authorizes based on one set of reads and then deletes in a separate statement keyed only by
id. If the row ownership/artist mapping — or the caller’s artist/org access — changes between those operations, the request can still delete a task it is no longer allowed to remove. Please move the authorization + delete into one DB-side operation so the permission check and mutation happen on the same snapshot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/scheduled-actions/delete/route.ts` around lines 37 - 63, Replace the separate select-then-delete with a single DB-side conditional delete so authorization and mutation occur atomically: execute one supabase delete on "scheduled_actions" with .eq("id", id) plus a conditional that enforces (account_id = accountId OR artist_account_id IN (the set of artist accounts the caller can act for)), then check the returned count/rows to decide 404 vs 403; remove the canDeleteAsOwner / canDeleteAsArtistAccess flow and the prior select, and if you currently rely on checkAccountArtistAccess to compute permitted artist ids, move that logic into a DB-safe filter (e.g., a subquery or join) so the delete only succeeds when the DB snapshot proves permission.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/VercelChat/ViewingMessage.tsx`:
- Line 29: The wrapping div in ViewingMessage.tsx was changed to always include
"items-end", which causes assistant/system messages to right-align and breaks
the layout contract (see MessageParts.tsx where assistant uses justify-start and
user justify-end); restore conditional alignment by using the cn() utility to
merge classes and apply "items-end" only when the message role indicates a user
(e.g., message.role === 'user'), otherwise use "items-start"/no end alignment so
assistant/system messages remain left-aligned; update the className on the
container div inside the ViewingMessage component accordingly to follow the
project's cn() convention.
In `@lib/tasks/deleteTask.ts`:
- Around line 22-30: deleteTaskFromDatabase currently ignores the fetch response
so callers may treat failures as success; update deleteTaskFromDatabase to
inspect the fetch Response, check response.ok and throw an Error for non-2xx
statuses, but if the intent is idempotent treat a 404 as success (do not throw
for 404). Use the existing function name deleteTaskFromDatabase and the
"/api/scheduled-actions/delete" response to decide: await fetch(...), then if
(!response.ok && response.status !== 404) throw a descriptive Error (include
status/text) so callers can handle/display errors instead of showing success to
the user.
---
Outside diff comments:
In `@app/api/scheduled-actions/delete/route.ts`:
- Around line 37-63: Replace the separate select-then-delete with a single
DB-side conditional delete so authorization and mutation occur atomically:
execute one supabase delete on "scheduled_actions" with .eq("id", id) plus a
conditional that enforces (account_id = accountId OR artist_account_id IN (the
set of artist accounts the caller can act for)), then check the returned
count/rows to decide 404 vs 403; remove the canDeleteAsOwner /
canDeleteAsArtistAccess flow and the prior select, and if you currently rely on
checkAccountArtistAccess to compute permitted artist ids, move that logic into a
DB-safe filter (e.g., a subquery or join) so the delete only succeeds when the
DB snapshot proves permission.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8238e07e-341c-4876-a7ae-796dea5d3f01
⛔ Files ignored due to path filters (1)
app/api/scheduled-actions/delete/__tests__/route.test.tsis excluded by!**/*.test.*and included byapp/**
📒 Files selected for processing (4)
app/api/scheduled-actions/delete/route.tscomponents/VercelChat/ViewingMessage.tsxhooks/useDeleteScheduledAction.tslib/tasks/deleteTask.ts
|
|
||
| return ( | ||
| <div className={cn("flex flex-col gap-2", { "items-end": message.role === "user" })}> | ||
| <div className="flex flex-col gap-2 items-end"> |
There was a problem hiding this comment.
Regression: Assistant messages will incorrectly align to the right.
Removing the conditional items-end breaks the conversation layout. Per MessageParts.tsx (lines 72-73), assistant messages use justify-start while user messages use justify-end. Hardcoding items-end here causes assistant/system messages to right-align, violating the established alignment contract.
Additionally, this change deviates from the project's convention of using cn() for conditional class merging.
🔧 Proposed fix to restore conditional alignment
- <div className="flex flex-col gap-2 items-end">
+ <div className={cn("flex flex-col gap-2", { "items-end": message.role === "user" })}>Based on learnings: "Use cn() utility for class merging in React components."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className="flex flex-col gap-2 items-end"> | |
| <div className={cn("flex flex-col gap-2", { "items-end": message.role === "user" })}> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/VercelChat/ViewingMessage.tsx` at line 29, The wrapping div in
ViewingMessage.tsx was changed to always include "items-end", which causes
assistant/system messages to right-align and breaks the layout contract (see
MessageParts.tsx where assistant uses justify-start and user justify-end);
restore conditional alignment by using the cn() utility to merge classes and
apply "items-end" only when the message role indicates a user (e.g.,
message.role === 'user'), otherwise use "items-start"/no end alignment so
assistant/system messages remain left-aligned; update the className on the
container div inside the ViewingMessage component accordingly to follow the
project's cn() convention.
| async function deleteTaskFromDatabase(taskId: string, accessToken: string): Promise<void> { | ||
| await fetch("/api/scheduled-actions/delete", { | ||
| method: "DELETE", | ||
| headers: { "Content-Type": "application/json" }, | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| Authorization: `Bearer ${accessToken}`, | ||
| }, | ||
| body: JSON.stringify({ id: taskId }), | ||
| }); |
There was a problem hiding this comment.
Do not swallow fallback delete failures.
deleteTaskFromDatabase() ignores the response status, so any 401/403/404/500 from /api/scheduled-actions/delete is treated as success. With the new protected route, that means the caller can reach toast.success(...) even though nothing was deleted. Check response.ok here and throw on non-2xx; if this delete is meant to be idempotent, special-case 404 explicitly instead of swallowing every failure.
Suggested fix
async function deleteTaskFromDatabase(taskId: string, accessToken: string): Promise<void> {
- await fetch("/api/scheduled-actions/delete", {
+ const response = await fetch("/api/scheduled-actions/delete", {
method: "DELETE",
headers: {
"Content-Type": "application/json",
Authorization: `Bearer ${accessToken}`,
},
body: JSON.stringify({ id: taskId }),
});
+
+ if (!response.ok) {
+ const errorText = await response.text();
+ throw new Error(`HTTP ${response.status}: ${errorText}`);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function deleteTaskFromDatabase(taskId: string, accessToken: string): Promise<void> { | |
| await fetch("/api/scheduled-actions/delete", { | |
| method: "DELETE", | |
| headers: { "Content-Type": "application/json" }, | |
| headers: { | |
| "Content-Type": "application/json", | |
| Authorization: `Bearer ${accessToken}`, | |
| }, | |
| body: JSON.stringify({ id: taskId }), | |
| }); | |
| async function deleteTaskFromDatabase(taskId: string, accessToken: string): Promise<void> { | |
| const response = await fetch("/api/scheduled-actions/delete", { | |
| method: "DELETE", | |
| headers: { | |
| "Content-Type": "application/json", | |
| Authorization: `Bearer ${accessToken}`, | |
| }, | |
| body: JSON.stringify({ id: taskId }), | |
| }); | |
| if (!response.ok) { | |
| const errorText = await response.text(); | |
| throw new Error(`HTTP ${response.status}: ${errorText}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/tasks/deleteTask.ts` around lines 22 - 30, deleteTaskFromDatabase
currently ignores the fetch response so callers may treat failures as success;
update deleteTaskFromDatabase to inspect the fetch Response, check response.ok
and throw an Error for non-2xx statuses, but if the intent is idempotent treat a
404 as success (do not throw for 404). Use the existing function name
deleteTaskFromDatabase and the "/api/scheduled-actions/delete" response to
decide: await fetch(...), then if (!response.ok && response.status !== 404)
throw a descriptive Error (include status/text) so callers can handle/display
errors instead of showing success to the user.
| const auth = await validateHeaders(req); | ||
| if (auth instanceof Response) { | ||
| return auth; | ||
| } |
There was a problem hiding this comment.
Why are you updating the chat codebase to fix an issue with an API endpoint?
| const { data: scheduledAction, error: selectError } = await supabase | ||
| .from("scheduled_actions") | ||
| .select("id, account_id, artist_account_id") | ||
| .eq("id", id) | ||
| .maybeSingle(); |
There was a problem hiding this comment.
SRP - supabase libs in lib/supabase
- no supabase libs should exist in the
chatcodebase.
- Replaced direct Supabase calls in the DELETE route with dedicated functions for selecting and deleting scheduled actions. - Introduced `selectScheduledActionById` and `deleteScheduledActionById` utility functions for improved code organization and readability. - Updated tests to mock new utility functions, ensuring proper validation and error handling during deletion process.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/api/scheduled-actions/delete/route.ts (1)
64-68: Log the failure before returning 500.The catch block collapses select/auth/delete failures into the same response without any server-side signal, which will make production regressions here hard to diagnose. Emit a structured error log before returning the generic 500.
As per coding guidelines, "For API routes, ensure: Proper error handling and validation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/scheduled-actions/delete/route.ts` around lines 64 - 68, The catch block in app/api/scheduled-actions/delete/route.ts currently swallows the error and returns NextResponse.json({ error: "Failed to delete task from database" }, { status: 500 }); — modify that catch to capture the thrown error (e.g., catch (err)), emit a structured server-side log before returning (use your project's logger or console.error) that includes the error object and relevant context such as the task id and auth/user info available to the route handler, and then return the same NextResponse.json; reference the catch block and the NextResponse.json call so you update the correct spot.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/supabase/scheduled_actions/deleteScheduledActionById.ts`:
- Around line 3-4: The deleteScheduledActionById function performs a blind
delete by id which can race with concurrent permission changes; change its
signature to accept the caller's user id (e.g., add a userId: string parameter)
and make the deletion atomic by adding the ownership filter in the same DB call
(use supabase.from("scheduled_actions").delete().eq("id", id).eq("owner_id",
userId)); alternatively enforce via DB RLS in the same statement, but ensure the
permission check is performed server-side inside deleteScheduledActionById
rather than via a prior read.
---
Nitpick comments:
In `@app/api/scheduled-actions/delete/route.ts`:
- Around line 64-68: The catch block in
app/api/scheduled-actions/delete/route.ts currently swallows the error and
returns NextResponse.json({ error: "Failed to delete task from database" }, {
status: 500 }); — modify that catch to capture the thrown error (e.g., catch
(err)), emit a structured server-side log before returning (use your project's
logger or console.error) that includes the error object and relevant context
such as the task id and auth/user info available to the route handler, and then
return the same NextResponse.json; reference the catch block and the
NextResponse.json call so you update the correct spot.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8237db7c-ae31-4fda-b88d-d18bd9a39e37
⛔ Files ignored due to path filters (1)
app/api/scheduled-actions/delete/__tests__/route.test.tsis excluded by!**/*.test.*and included byapp/**
📒 Files selected for processing (3)
app/api/scheduled-actions/delete/route.tslib/supabase/scheduled_actions/deleteScheduledActionById.tslib/supabase/scheduled_actions/selectScheduledActionById.ts
| export async function deleteScheduledActionById(id: string) { | ||
| return supabase.from("scheduled_actions").delete().eq("id", id); |
There was a problem hiding this comment.
Make the delete authorization atomic.
app/api/scheduled-actions/delete/route.ts authorizes first and then calls this id-only delete, so a concurrent ownership/access change between those two steps can still remove a row the caller no longer controls. For a destructive path, push the permission check into the delete itself (for example via RLS or a single DB-side operation) instead of doing a separate pre-read.
As per coding guidelines, "Implement built-in security practices for authentication and data handling."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/supabase/scheduled_actions/deleteScheduledActionById.ts` around lines 3 -
4, The deleteScheduledActionById function performs a blind delete by id which
can race with concurrent permission changes; change its signature to accept the
caller's user id (e.g., add a userId: string parameter) and make the deletion
atomic by adding the ownership filter in the same DB call (use
supabase.from("scheduled_actions").delete().eq("id", id).eq("owner_id",
userId)); alternatively enforce via DB RLS in the same statement, but ensure the
permission check is performed server-side inside deleteScheduledActionById
rather than via a prior read.
Summary by CodeRabbit
New Features
Bug Fixes